Skip to content

fix(library): read footprint and lib-table fields from the parsed tree - #322

Open
pauliuszaleckas wants to merge 1 commit into
mixelpixx:mainfrom
pauliuszaleckas:fix/library-fields-from-parsed-tree
Open

fix(library): read footprint and lib-table fields from the parsed tree#322
pauliuszaleckas wants to merge 1 commit into
mixelpixx:mainfrom
pauliuszaleckas:fix/library-fields-from-parsed-tree

Conversation

@pauliuszaleckas

Copy link
Copy Markdown
Contributor

Summary

get_footprint_info and parse_lib_table read their string fields by scanning
the raw file text instead of the S-expression they had already parsed. Two
user-visible consequences:

  • An escaped quote truncates the value. KiCad writes a description of
    0.1" pitch headers as (descr "0.1\" pitch headers"); the scan stopped at
    the escaped quote and returned 0.1\.
  • A field written across a line break is missed entirely, and the entry comes
    back blank — an fp-lib-table entry loses its nickname, URI and type, so its
    libraries stop resolving.

Not linked to an issue. #84 tracks the same root cause elsewhere but explicitly
excludes parse_lib_table, whose indentation scan was already fixed; this is
the field scraping that remained inside each located block.

Approach

extract_sexp_string looked for the literal (key " and read to the next ":

let pat = format!("({} \"", key);
let start = block.find(&pat)? + pat.len();
let end = block[start..].find('"')? + start;

It has no notion of escapes, of nesting, or of any whitespace other than one
space. Both call sites now read from the parsed tree — find_str(tag) for a
tagged field, and the root node's datum for a footprint's name — so escape
decoding and whitespace tolerance come from the parser. The function had no
callers left and is deleted.

Two get_footprint_info hunks are behaviour-preserving and included only for
consistency: find_all("pad").len() becomes an explicit direct-child filter
(find_all is already direct-children-only), and !find_all("model").is_empty()
becomes find("model").is_some(), which short-circuits and allocates nothing.

Why lib-table entries are still located textually. The obvious move is
parse_sexp over the whole file. parse_lib_table is infallible by design —
every caller path (read_flat_lib_tableflatten_lib_table) degrades to an
empty list and a tracing::warn! rather than surfacing an error — so a
whole-file parse would let one malformed entry silently discard every library
in the table, which for this path means footprint and symbol resolution failing
wholesale. Keeping find_block_starts + find_balanced_block (both already
string-aware and indentation-agnostic) preserves per-entry tolerance while still
removing the scraping. A malformed entry is now skipped with a warning naming
its offset.

Compatibility and safety

No public compatibility impact. No tool, argument, schema, config key or path
changes. Read paths only — nothing here writes a file or touches IPC.

get_footprint_info's name and description response fields can now return a
correct value where they previously returned a truncated or empty one; no key
is added, removed or renamed.

Validation

$ cargo fmt --all -- --check
$ cargo test --workspace --locked --lib --tests
$ cargo test --workspace --locked --doc
$ cargo clippy --workspace --locked -- -D warnings

All four clean, run against this branch on top of main (not merely on the
development branch this was cherry-picked from).

The two bug-fix tests were verified to fail against the old scraper before the
fix was restored:

parse_lib_table_keeps_an_escaped_quote_in_a_description ... FAILED
parse_lib_table_reads_a_field_split_across_lines ... FAILED

parse_lib_table_skips_only_the_entry_that_does_not_parse passes both before
and after — it is not a regression test for a fixed bug, but a guard on the
tolerance property against the whole-file-parse refactor described above. Its
comment says so.

Not run: viewer, plugin, packaging and real-KiCad checks. This touches one file
in konnect-core and none of those paths.

  • cargo fmt --all -- --check
  • cargo test --workspace --locked --lib --tests (what CI runs)
  • cargo test --workspace --locked --doc
  • cargo clippy --workspace --locked -- -D warnings
  • Relevant viewer, plugin, packaging, and real-KiCad checks — not applicable

Review checklist

  • The diff is focused and contains no generated output, personal data, or unrelated cleanup.
  • New names follow docs/NAMING_CONVENTIONS.md; public renames include compatibility handling.
  • New behavior and failure paths have regression coverage.
  • File mutations are atomic and preserve unrelated content — read paths only, nothing written.
  • IPC mutations verify the requested board and do not leave partial batches — no IPC.
  • If tools were added/removed: counts and docs updated — no tools added or removed.

@mixelpixx

Copy link
Copy Markdown
Owner

Holding merge on the two Windows-only CI failures: flatten_lib_table_follows_nested_table_entries and list_footprint_libraries_expands_a_nested_table_of_env_var_uris. The core escape/whitespace fix reviews correct — it's specifically the nested Table-type / env-var URI expansion path that regressed on Windows. Everything else is MERGE-ready once those two are green.

get_footprint_info parsed the footprint for pads, courtyard and model, then
still scraped the name and description out of the raw source. parse_lib_table
scraped all four of its fields the same way.

The scan looked for the literal `(key "` and read to the next quote, so an
escaped quote truncated the value — a descr of `0.1" pitch` came back as
`0.1\` — and a field written across a line break was missed entirely, leaving
the entry blank.

Lib-table entries are still located textually before being parsed. The
function is infallible by design, and a whole-file parse would let one
malformed entry discard every library in the table.

The lib-table test helper escapes the values it interpolates, the way KiCad
and quote_lib_table_string write them. It interpolated a tempdir path raw, so
on Windows the nested table's `\template-fp-lib-table` decoded with a TAB and
the entry pointed at a file that does not exist — the two Windows-only
failures on this branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pauliuszaleckas
pauliuszaleckas force-pushed the fix/library-fields-from-parsed-tree branch from 0120b5b to 85f3edb Compare August 26, 2026 12:24
@pauliuszaleckas

Copy link
Copy Markdown
Contributor Author

Both Windows failures were the test fixture, not the fix.

kicad_style_table interpolated a tempdir path into the table raw. On Windows that path holds backslashes, and the nested table is named template-fp-lib-table, so the file text contained …\.tmpXXXX\template-fp-lib-table — the parser reads \t as a TAB, exactly as KiCad does, so the URI pointed at a file that does not exist and the (type "Table") entry expanded to nothing. Not reachable through Konnect on a real table: register_*_library writes URIs through portable_uri (forward slashes) and quote_lib_table_string (escaped), and KiCad escapes them too.

The helper now escapes what it interpolates via escape_library_string, the same way quote_lib_table_string writes it, so the fixture is what KiCad would have written on either platform.

Verified rather than assumed: pointing TMPDIR at a directory containing \t reproduces both failures verbatim on Linux (nested table not followed: [], nested table not expanded: {"count":0,"libraries":[]}) and both pass with the fix. Added parse_lib_table_reads_an_escaped_windows_uri as a platform-independent guard on the property the helper now relies on.

Full gate green: 602 lib/tests, doctests, clippy -D warnings, fmt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants